-
-
Notifications
You must be signed in to change notification settings - Fork 93
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add package @preact/signals-react-transform
#375
Conversation
🦋 Changeset detectedLatest commit: 255208a The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for preact-signals-demo ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Size Change: +267 B (0%) Total Size: 78 kB
ℹ️ View Unchanged
|
karma.conf.js
Outdated
@@ -283,7 +287,7 @@ module.exports = function (config) { | |||
jsx: "preserve", | |||
|
|||
// esbuild options | |||
target: downlevel ? "es5" : "es2015", | |||
target: downlevel ? "es5" : "es2019", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops - will undo this. Did this to not downlevel async/await while debugging tests
@@ -0,0 +1,119 @@ | |||
# Signals React Transform |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsure what's the right level of detail here. Would love other's thoughts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's perfectly fine. We could theoretically shorten the intro paragraph about what signals are by just pointing to the blog post. But then again that I think leaving as is, is fine too.
// TODO: | ||
// - how to trigger rerenders on attributes change if transform never sees | ||
// `.value`? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still need to build the signal attribute handling in our jsx handler
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do this in a follow up PR. this one is big enough 😅
// TODO: Consider alternate implementation, where on enter of a function | ||
// expression, we run our own manual scan the AST to determine if the | ||
// function uses signals and is a component. This manual scan once upon | ||
// seeing a function would probably be faster than running an entire | ||
// babel pass with plugins on components twice. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For anyone with more experience with babel transforms - if I want to know if a function body contains JSX to know whether to perform my transformation, is it better to do a local visitor pass inside this ArrowFunctionExpression handler to observe that (only running my local visitor), and then transform? Or to do the observation in the larger babel visitor pass and then transform after, which triggers and additional visitor pass with all plugins?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good question. I've never thought about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use something like this on a VariableDeclaration to see whether they are used as JSX
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh interesting. IIUC, this code looks to see if a particular identifier is used as JSX somewhere in the same file?
|
What if someone calls |
…to make nested hook and component signal usage work
Hooks that use signals are transformed to call |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay! I believe this is now ready for review 🎉
// TODO: | ||
// - how to trigger rerenders on attributes change if transform never sees | ||
// `.value`? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do this in a follow up PR. this one is big enough 😅
return hasLeadingComment(path, optOutCommentIdentifier); | ||
} | ||
|
||
function isOptedIntoSignalTracking(path: NodePath | null): boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and in isOptedOutOfSignalTracking
, I'm climbing up the AST to find the most logical place for a comment to live. For example supporting a use case such as:
/** @trackSignals */
export default function useCustomHook() {}
return ( | ||
fnNameStartsWithCapital(path) && // Function name indicates it's a component | ||
getData(path.scope, containsJSX) === true && // Function contains JSX | ||
path.scope.parent === path.scope.getProgramParent() // Function is top-level |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about high order components? This type of components can use signals too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, it's true. I'm thinking HoC will need to manually opt-in using /** @trackSignals */
or by calling useSignals()
directly in their wrapper function.
It is harder to detect HoCs and know where to put the useSignals call so for now I'm explicitly skipping them. I should add a note to the Readme calling this out for people who want to use signals in higher order components.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it a good idea to mention edge cases in readme
62c0fbd
to
190d543
Compare
190d543
to
dd070c9
Compare
signal.value; | ||
return <div>Hello World</div>; | ||
} finally { | ||
_effect.f(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any opinions on the name of this method f
? It's short for finishEffect
to stop the current effect.
I've changed the return of useSignals
from an method to an object that can hang other stuff of like the dispose symbol for using with using
which this PR also includes support for.
a29b9e3
to
cd3a22d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks amazing
// TODO: Consider alternate implementation, where on enter of a function | ||
// expression, we run our own manual scan the AST to determine if the | ||
// function uses signals and is a component. This manual scan once upon | ||
// seeing a function would probably be faster than running an entire | ||
// babel pass with plugins on components twice. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use something like this on a VariableDeclaration to see whether they are used as JSX
This PR adds a new package
@preact/signals-react-transform
that provides a Babel plugin to transform functions to observer/track what signals are used when the function runs. This plugin is the start to an alternate React integration for signals that does not hook into React internals. Once our approach here is finalized and published, I'll update the@preact/signals-react
package to reference this and provide various options for using signals in React.Check the test file for examples of what the transformation looks like.
I'm currently exploring two different kinds of transformations in this implementation to see which one works better. One just adds a call to
useSignals()
to the top of the component and relies on the next invocation ofuseSignals
or a microtick to stop tracking signals. The other wraps the component body in atry/finally
to start and stop tracking signals. I'm currently leaning towards making thetry/finally
the default cuz it leaves less room for tricky edge cases that may lead to non-obvious behavior and rerenders.However, the implementation of
useSignals
is the same either way. We'll always setup the microtick to clean up any outstanding effects to prevent any loose ends regardless of which transformation is used. So users can manually dropuseSignals
into a component and get basic functionality.